feat: implement NEP264, function call gas weight #6285

austinabell · 2022-02-12T00:57:46Z

More of a rough draft because I don't have context around the runtime as to the best migration plan of these APIs. I've followed the recommendation of #6150 to avoid refactoring to move action receipts to VMLogic, but this means the External trait needs to be updated, and it's unclear if anyone has preferences.

TODO:

e2e test to cover RuntimeExt as current tests just cover MockedExternal (same functionality though)
- Also these tests don't check that gas was distributed to the correct function calls
Determine the best path for trait migration (would we do the refactor mentioned above before any releases?)
Where does the protocol version number come from, and what should this one be numbered?
Benchmark to gauge gas cost and verify existing impl doesn't have higher cost
Would be nice to fuzz test this to make sure there aren't any weird edge cases

runtime/near-vm-logic/src/dependencies.rs

runtime/near-vm-logic/src/logic.rs

MaksymZavershynskyi · 2022-02-14T03:01:07Z

runtime/runtime/src/ext.rs

+}
+
+#[cfg(feature = "protocol_feature_function_call_ratio")]
+struct GasRatioMetadata {


We try to avoid u* types in the runtime as much as possible. If you find usage of u* type anywhere please file a Github issue.

I must say I don't entirely understand what should be used instead. As this interfaces with VmLogic which used raw u64/u32, we actually have a whole bunch of u64s in various signatures in this module.

But yeah, I'd probably spell this as:

struct RuntimeExt { ... gas_weights: Vec<(FunctionCallActionIndex, GasWeight)> } struct GasWeight(u64); struct FunctionCallActionIndex { receipt_index: usize, action_inde } impl RuntimeExt { fn get_function_call_action_mut(&mut self, index: FunctionCallActionIndex) -> &mut FunctionCallAction { } }

Separate the two-dimensional index into its own type

Add a typed wrapper to the weight

Remove gas_ratio_sum field -- it seems we can compute it in the distribute function. To check if it's zero, you can check if gas_weights is not empty

Do you want the u64 removed from the trait interface or just internally in the implementation? I notice for the other types in the signature the types are aliases in the trait definition but then just use the primitive type in the implementation.

As for the proposed API, it wouldn't work with the current implementation because the gas weights need to be borrowed mutably so I can't take a mutable reference on self when it's used. Seems more complex to read with this change to me, but happy to change to whatever is preferred.

So I've changed to just wrapping internally for now, but can expose this type through the trait interface if that's what we want to move the API toward

runtime/near-vm-logic/src/dependencies.rs

MaksymZavershynskyi · 2022-02-14T03:24:20Z

runtime/runtime/src/ext.rs

+                        panic!("Invalid index for assigning unused gas ratio");
+                    }
+
+                    assign_gas


It is preferable if we always distribute all of the unused gas to avoid emitting gas refund receipt. It is okay if the last receipt gas a little bit extra gas. Right now we have leftover gas because of the division. We then would need to write tests to make sure refund receipt is never emitted. Once we do it the return value is always going to be either equal to gas or zero, which means it will become semantically a bool.

👍 to distributing all gas, but let's do it evenly, rather than by just tacking extra onto the last receipt, as that can be very unbalanced. If you distribute 9 gas among five calls, you want to get [2, 2, 2, 2, 1] rathre than [1, 1, 1, 1, 5]

The only issue I see to this is the overhead of getting the modulus of the division before the operations or having to re-index the initial gas % weight_sum items if we want to micro optimize. I will do this change and then will revisit later to see if implications on the cost of the operation.

Also, it's unclear how the behaviour would work in the cases of a few weights with very large values. For example, if there are two with weights of 1000 and 2000 and only 2500 gas and the remainder would be the whole 2500, would the gas be split evenly among the two? Maybe we don't have to worry about this kind of case, but want to be clear.

Also, balancing it among the first transactions might not be as important as the last one(s), which is commonly callback. Opinionated take but I think in a lot of cases it's better to make sure the final callback has enough gas.

Made changes with Max's suggestion for now for simplicity, but will update once we resolve these edge cases :)

matklad

Feels broadly right to me!

One thing which I feel suuuper uneasy though is the fact that the tests here check our mock implementation, rather than the real one. Here's what we can do here:

Have at least one integration test here. I don't know what's the right place for it though. They are some more integration tests in near-vm-runner/tests/rs_contract, but they still use mocked external
Move promise handling logic from runtime to vm-logic. Today vm-logic depends on primitives, so that should be fine. It'll also allow us to reduce duplication in MockedExternal.
Alternatively, keep actual promises data in runtime, but move bookeeping to vm-logic

Though, I'd personally be fine langing this PR without proper tests for new functionality, as long as the feature is experimental. That is, we absolutelly have to add tests which exercise the real thing, but we can do that in a follow-up PR

matklad · 2022-02-17T11:44:04Z

runtime/near-vm-logic/src/dependencies.rs

+    ///
+    /// Panics if the `receipt_index` does not refer to a known receipt.
+    #[cfg(feature = "protocol_feature_function_call_ratio")]
+    // TODO: unclear if this should be a supertrait to avoid semver breaking changes


We intentionally don't have any semver guarantees for nearcore crates, so, no, you can just change the code.

so are you suggesting modifying append_action_function_call or keeping as is?

The current implementation is fine. Please remove TODO here

runtime/near-vm-logic/src/logic.rs

runtime/near-vm-runner/src/imports.rs

matklad · 2022-02-17T12:10:21Z

runtime/runtime/src/ext.rs

+}
+
+#[cfg(feature = "protocol_feature_function_call_ratio")]
+struct GasRatioMetadata {


I must say I don't entirely understand what should be used instead. As this interfaces with VmLogic which used raw u64/u32, we actually have a whole bunch of u64s in various signatures in this module.

But yeah, I'd probably spell this as:

struct RuntimeExt { ... gas_weights: Vec<(FunctionCallActionIndex, GasWeight)> } struct GasWeight(u64); struct FunctionCallActionIndex { receipt_index: usize, action_inde } impl RuntimeExt { fn get_function_call_action_mut(&mut self, index: FunctionCallActionIndex) -> &mut FunctionCallAction { } }

Separate the two-dimensional index into its own type

Add a typed wrapper to the weight

Remove gas_ratio_sum field -- it seems we can compute it in the distribute function. To check if it's zero, you can check if gas_weights is not empty

matklad · 2022-02-17T12:18:00Z

runtime/runtime/src/ext.rs

+            self.gas_ratio_sum =
+                self.gas_ratio_sum.checked_add(gas_ratio).ok_or(HostError::IntegerOverflow)?;


This error condition doesn't feel great to me. This is a code path which could be triggered by the user, it's not just dead code, so it adds to the number of code paths which we need to exercise. "You can't use u64::MAX - 1 as a ratio" is also an error condition the user needs to worry about. Let's implement the logic in a way which can deal with arbitrary ratios. Using u128 to sum the ratios should work I think.

Seems like a bit of a foot-gun because the gas maximum is u64::MAX, so having the ratio larger doesn't do anything and goes into the fallback case, which could silently cause unexpected behaviour. Should never realistically happen so maybe not worth handling this case.

matklad · 2022-02-17T12:25:31Z

runtime/runtime/src/ext.rs

+                        panic!("Invalid index for assigning unused gas ratio");
+                    }
+
+                    assign_gas


👍 to distributing all gas, but let's do it evenly, rather than by just tacking extra onto the last receipt, as that can be very unbalanced. If you distribute 9 gas among five calls, you want to get [2, 2, 2, 2, 1] rathre than [1, 1, 1, 1, 5]

runtime/near-vm-logic/src/tests/gas_counter.rs

austinabell · 2022-02-18T23:44:22Z

Move promise handling logic from runtime to vm-logic. Today vm-logic depends on primitives, so that should be fine. It'll also allow us to reduce duplication in MockedExternal.

Alternatively, keep actual promises data in runtime, but move bookeeping to vm-logic

You indicated in the issue that you thought this refactor should be done in a separate PR, has your opinion on this changed or are you referring to doing this after this PR?

austinabell · 2022-02-28T21:34:14Z

Okay I'm going to open this PR for review so I can start getting other feedback while I wait for resolution on a few things or input on those things.

The outstanding things are:

Where does the protocol version number come from? I picked the next number I could see, but not sure if it's overlapping
Move promise handling logic from runtime to vm-logic based on a suggestion from @matklad. I was originally avoiding this based on NEP 264 tracking issue #6150 recommending doing this in a separate PR, unclear if the plan is changed and now is intended to do in this one.
e2e test to cover RuntimeExt. Scoping this out it seems like this is more involved since there isn't the test scaffolding set up to allow testing these, and maybe not that necessary if logic is moved from the previous point
Should the GasWeight type be exposed to the trait API? feat: implement NEP264, function call gas weight #6285 (comment) and related comments suggested this, but it is unlike other types in related files that are aliases of the primitive types. I've made the newtype part of the API here 78d4a95 (#6285) but I don't think it's very consistent if we don't update others.

core/primitives/src/version.rs

bowenwang1996 · 2022-03-01T22:53:34Z

runtime/near-vm-logic/src/dependencies.rs

+    ///
+    /// Panics if the `receipt_index` does not refer to a known receipt.
+    #[cfg(feature = "protocol_feature_function_call_ratio")]
+    // TODO: unclear if this should be a supertrait to avoid semver breaking changes


The current implementation is fine. Please remove TODO here

bowenwang1996 · 2022-03-01T23:05:40Z

runtime/runtime/src/ext.rs

+            let mut distribute_gas = |metadata: &FunctionCallActionIndex, assigned_gas: u64| {
+                let FunctionCallActionIndex { receipt_index, action_index } = metadata;
+                if let Some(Action::FunctionCall(FunctionCallAction { ref mut gas, .. })) = self
+                    .action_receipts
+                    .get_mut(*receipt_index)
+                    .and_then(|(_, receipt)| receipt.actions.get_mut(*action_index))
+                {
+                    *gas += assigned_gas;
+                } else {
+                    panic!("Invalid index for assigning unused gas weight");
+                }
+            };
+
+            let distributed: u64 = self
+                .gas_weights
+                .iter()
+                .map(|(action_index, GasWeight(weight))| {
+                    let assigned_gas = gas_per_weight * weight;
+
+                    distribute_gas(action_index, assigned_gas);
+
+                    assigned_gas
+                })
+                .sum();


I suggest rewriting this logic using a for loop to make it easier to understand

bowenwang1996 · 2022-03-01T23:08:10Z

runtime/near-vm-logic/src/logic.rs

+    pub fn outcome(mut self) -> VMOutcome {
+        #[cfg(feature = "protocol_feature_function_call_weight")]
+        if !self.context.is_view() {
+            // Distribute unused gas to scheduled function calls
+            let unused_gas = self.context.prepaid_gas - self.gas_counter.used_gas();
+
+            // Distribute the unused gas and prepay for the gas.
+            if self.ext.distribute_unused_gas(unused_gas) {
+                self.gas_counter.prepay_gas(unused_gas).unwrap();
+            }
+        }


This function now has a nontrivial side effect. We might want to rename it and add a comment about what it does

took a stab at this, open to any suggestions of naming or any other information to include in the rustdocs since it's opinionated

austinabell · 2022-03-02T00:27:55Z

Also, I forgot to ask one other thing: the hash for the integration tests changed because I updated the protocol version number (which seems to be serialized into the block hash). So I didn't want to just update the test to pass before checking if any other implications should be considered.

Is there anything other than updating those hashes to match that needs to be done?

mm-near · 2022-03-08T12:53:46Z

runtime/near-vm-logic/src/dependencies.rs

@@ -276,6 +278,49 @@ pub trait External {
        prepaid_gas: Gas,
    ) -> Result<()>;

+    /// Attach the [`FunctionCallAction`] action to an existing receipt.
+    ///


I'd mention that this is the same function as above (append_action_function_call) with the added argument.

And maybe in this documentation focus a little bit more on what 'gas_weight' means.

And what happens if I set both 'prepaid_gas' and gas_weight?

and what if gas_weight is 0 ? does it still include gas then ?

Assumed the docs for this would be lighter since it's documented in promise_batch_action_function_call_weight, but I see the benefit in giving some info here also.

it is fine to have lighter docs - but then let's make sure that we have a 'pointer' to the place where people can read more details.

(I actually don't know which file is read by users - this one or logic.rs?)

mm-near · 2022-03-08T13:04:06Z

runtime/near-vm-logic/src/mocks/mock_external.rs

@@ -209,6 +248,46 @@ impl External for MockedExternal {
    fn validator_total_stake(&self) -> Result<Balance> {
        Ok(self.validators.values().sum())
    }
+
+    #[cfg(feature = "protocol_feature_function_call_weight")]
+    fn distribute_unused_gas(&mut self, gas: Gas) -> GasDistribution {


comment (especially the part that you're actually modifying the assigned_gas for the receipts

This is documented on the trait. Are you suggesting all implementations to give details about how specifically it distributes gas?

yes, if possible. It helps a lot during code review and later code reading.

mm-near · 2022-03-08T13:05:53Z

runtime/near-vm-logic/src/mocks/mock_external.rs

+                {
+                    *gas += assigned_gas;
+                } else {
+                    panic!("Invalid index for assigning unused gas weight");


maybe also print what was the index (and also some other identifier - transaction/receipt id?)

( I assume that this should never ever happen, right?)

yeah, this is currently unreachable, but just have a static error message to help debug if the functionality ever changes to re-order actions or receipts.

Using formatting machinery to include indices comes at a cost, can you confirm this is ideal before I make the change?

but wait - this cost would be paid only if the panic actually triggers, right?

yeah, I was more referring to compiled code size. I'll just add it in. I've been wired to be a little too cautious about this

mm-near · 2022-03-08T13:09:35Z

runtime/near-vm-logic/src/mocks/mock_external.rs

+        let gas_weight_sum: u128 =
+            self.gas_weights.iter().map(|(_, GasWeight(weight))| *weight as u128).sum();
+        if gas_weight_sum != 0 {
+            let gas_per_weight = (gas as u128 / gas_weight_sum) as u64;


if someone sets very large 'gas_weight' - then gas_per_weight can be 0, right?
(we should mention it in the comment)

Added docs in vm logic for this, let me know if it's preferable to duplicate docs to the external trait and/or impls

Thanks for adding those. I'd also add a one-line comment here, that we're using a floor, and we know that it might be 0 if the sum is too large.

mm-near · 2022-03-08T13:14:22Z

runtime/near-vm-logic/src/tests/gas_counter.rs

+    }
+    let outcome = logic.compute_outcome_and_distribute_gas();
+
+    // Verify that all gas is used when only one ratio is specified


confusing comment

ah, yes, this is also an outdated assertion since now all gas is used. Updated

runtime/near-vm-logic/src/tests/gas_counter.rs

runtime/runtime/src/ext.rs

austinabell · 2022-03-08T17:18:24Z

@mm-near I've tried again to find a clean way to assert the amount of gas allocated with the current testing scaffolding based on your comments, but I'll try to point out why it's more involved and should come in a separate change:

External in VM logic is &'a mut dyn External so it would require downcasting just to get a MockedExternal to assert specific values on a mock implementation (doesn't seem that valuable)
- Would also be asserting specific values assuming gas usage on a mocked implementation. This also doesn't seem valuable and is very brittle for this test breaking for other reasons unless the test is involved and recalculates everything
There is no testing scaffolding to test non-mock implementations of External. This would be much easier and more valuable if testing after migration of receipt logic from external to VM logic
- Quite an involved change for this, and didn't want to block this implementation

If you think this is imperative to test specific allocated values in this PR I can do the suggestion above with downcasting and re-calculating gas allocations, but I wanted to be clear about context before doing this as it feels janky to me to do in this PR.

mm-near · 2022-03-09T20:06:18Z

runtime/near-vm-logic/src/dependencies.rs

@@ -276,6 +278,49 @@ pub trait External {
        prepaid_gas: Gas,
    ) -> Result<()>;

+    /// Attach the [`FunctionCallAction`] action to an existing receipt.
+    ///


it is fine to have lighter docs - but then let's make sure that we have a 'pointer' to the place where people can read more details.

(I actually don't know which file is read by users - this one or logic.rs?)

mm-near · 2022-03-09T20:09:37Z

runtime/near-vm-logic/src/mocks/mock_external.rs

@@ -209,6 +248,46 @@ impl External for MockedExternal {
    fn validator_total_stake(&self) -> Result<Balance> {
        Ok(self.validators.values().sum())
    }
+
+    #[cfg(feature = "protocol_feature_function_call_weight")]
+    fn distribute_unused_gas(&mut self, gas: Gas) -> GasDistribution {


yes, if possible. It helps a lot during code review and later code reading.

mm-near · 2022-03-09T20:13:16Z

runtime/near-vm-logic/src/mocks/mock_external.rs

+        let gas_weight_sum: u128 =
+            self.gas_weights.iter().map(|(_, GasWeight(weight))| *weight as u128).sum();
+        if gas_weight_sum != 0 {
+            let gas_per_weight = (gas as u128 / gas_weight_sum) as u64;


Thanks for adding those. I'd also add a one-line comment here, that we're using a floor, and we know that it might be 0 if the sum is too large.

mm-near · 2022-03-09T20:13:49Z

runtime/near-vm-logic/src/mocks/mock_external.rs

+                {
+                    *gas += assigned_gas;
+                } else {
+                    panic!("Invalid index for assigning unused gas weight");


but wait - this cost would be paid only if the panic actually triggers, right?

runtime/runtime/src/ext.rs

runtime/near-vm-logic/src/tests/gas_counter.rs

mm-near · 2022-03-09T20:16:48Z

runtime/near-vm-logic/src/tests/gas_counter.rs

+#[cfg(feature = "protocol_feature_function_call_weight")]
+#[test]
+fn function_call_weight_single_smoke_test() {
+    // Single function call


fine - but then let's have a test that actually test this new funcionality

runtime/near-vm-logic/src/tests/gas_counter.rs

mm-near · 2022-03-09T20:51:42Z

I'm approving to unblock you - but I'd really like to see a PR with a test that covers this functionality - as from what I see, if I change the logic to (for example) assign all the remaining gas to the last receipt (and ignore the weights) - all the tests would still pass..

austinabell · 2022-03-10T17:01:44Z

I'm approving to unblock you - but I'd really like to see a PR with a test that covers this functionality - as from what I see, if I change the logic to (for example) assign all the remaining gas to the last receipt (and ignore the weights) - all the tests would still pass..

I'm starting the refactor to allow this now. It makes me equally uncomfortable that this functionality isn't specifically tested and there is code duplication but seems janky to do in this PR and only would test the mock implementation.

feat: implement NEP264, function call gas ratio

1b5f49e

MaksymZavershynskyi reviewed Feb 14, 2022

View reviewed changes

matklad self-requested a review February 15, 2022 10:26

matklad reviewed Feb 17, 2022

View reviewed changes

austinabell added 4 commits February 18, 2022 12:38

adjust wording to weight instead of ratio, cleanup

fd91668

cleanup remaining ratio references

e012271

change logic to assign all unused gas

44044ff

wrap gas weight and fix clearing weights

ac4c2f0

austinabell added 2 commits February 18, 2022 23:40

add test for weight over u64 bounds

60d7293

Merge branch 'master' of github.com:near/nearcore into austin/nep264

886ebf6

matklad mentioned this pull request Feb 21, 2022

NEP 264 tracking issue #6150

Closed

austinabell changed the title ~~feat: implement NEP264, function call gas ratio~~ feat: implement NEP264, function call gas weight Feb 22, 2022

austinabell added 2 commits February 28, 2022 16:20

expose GasWeight newtype to trait interface

78d4a95

Merge branch 'master' into austin/nep264

43b2f92

austinabell marked this pull request as ready for review February 28, 2022 21:34

austinabell requested review from bowenwang1996, mm-near, olonho, nagisa, EgorKulikov and mzhangmzz as code owners February 28, 2022 21:34

austinabell requested review from matklad and MaksymZavershynskyi March 1, 2022 18:05

austinabell mentioned this pull request Mar 1, 2022

Implement NEP264 host function and refactor ext contract API near/near-sdk-rs#740

Closed

bowenwang1996 approved these changes Mar 1, 2022

View reviewed changes

austinabell added 2 commits March 1, 2022 19:19

addr comments

ab3e7ac

Merge branch 'master' into austin/nep264

0350337

austinabell added 7 commits March 2, 2022 13:17

fix lint ignore

6d0ec3a

switch vm logic function to have generic callback and gate by feature

0436ab6

Merge branch 'master' of github.com:near/nearcore into austin/nep264

1e88e76

fit lint and nightly hash for Protocol version bump

a979f6a

update gas distribution function return

56caf81

gate new types under feature flag

343741b

Make GasDistribution non_exhaustive

5aca615

austinabell mentioned this pull request Mar 4, 2022

feat: refactor ext API and add new NEP264 functionality near/near-sdk-rs#742

Merged

austinabell added 4 commits March 3, 2022 21:11

Merge branch 'master' into austin/nep264

50d3a13

Merge branch 'master' into austin/nep264

0f36da2

Merge branch 'master' into austin/nep264

bfb3c99

Merge branch 'master' into austin/nep264

e46f4a0

mm-near suggested changes Mar 8, 2022

View reviewed changes

austinabell added 4 commits March 8, 2022 11:49

addr docs comments

b713308

fmt

429c4d6

Merge branch 'master' of github.com:near/nearcore into austin/nep264

db092fb

update assert syntax

662a7f7

austinabell requested review from mm-near and removed request for olonho March 9, 2022 18:50

mm-near approved these changes Mar 9, 2022

View reviewed changes

austinabell added 2 commits March 10, 2022 11:58

address comments

bf069c1

Merge branch 'master' of github.com:near/nearcore into austin/nep264

0f6a8a2

matklad added the S-automerge label Mar 10, 2022

near-bulldozer bot merged commit 13057c5 into master Mar 10, 2022

near-bulldozer bot deleted the austin/nep264 branch March 10, 2022 20:08

austinabell mentioned this pull request Apr 4, 2022

refactor: Receipt handling to be done in VM logic #6468

Merged

austinabell mentioned this pull request Apr 14, 2022

runtime: Stabilise NEP264 #6564

Merged

		self.gas_ratio_sum =
		self.gas_ratio_sum.checked_add(gas_ratio).ok_or(HostError::IntegerOverflow)?;

feat: implement NEP264, function call gas weight #6285

feat: implement NEP264, function call gas weight #6285

Conversation

austinabell commented Feb 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell commented Feb 18, 2022

austinabell commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm-near commented Mar 9, 2022

austinabell commented Mar 10, 2022 • edited Loading

austinabell Feb 18, 2022 •

edited

Loading

austinabell commented Mar 10, 2022 •

edited

Loading